fix(grafana): correct curated-pack revision/hash pins, add maintainer verification script (#350) - #365
Conversation
shmsr
left a comment
shmsr
left a comment
There was a problem hiding this comment.
Review
Approve after rebase + a small leftover-docs fix. No UI testing — this does not change migrate/upload/Kibana behavior.
I live-ran python scripts/verify_curated_pack_pins.py against grafana.com: 6/6 pins verified, including 11835 re-pinned to revision 1. Offline tests (test_verify_curated_pack_pins.py + the new registry/manifest guards) passed. #362 is already on main, so the docs dependency is satisfied.
Please rebase onto current main before merge. Conflicts in docs/sources/grafana.md and tests/test_curated_packs.py. Also conflicts with #364 on grafana.md — land them sequentially.
Declining migrate-time hash checks against operator dashboards is the right call (a grafana.com download is not an instance export). Packs still apply by gnetId only; #362 is the per-panel net for dropped source metrics. Please don't close #350 as fully done without noting that remaining risk.
Please fix the leftover contradiction called out inline (Open Questions / file table still claim migrate-time drift warnings).
| `resolve_pack_for_dashboard` matches a curated pack purely by `gnetId` (with a | ||
| title/tags fallback) and applies it unconditionally — it never reads or | ||
| compares the incoming dashboard JSON's revision against the registry's | ||
| `gnet_revision`, and emits no drift warning. This is intentional, not an |
There was a problem hiding this comment.
This rewrite is accurate and matches the code (resolve_pack_for_dashboard does zero revision comparison).
Two leftovers later in this same file still claim the old behavior this section exists to retract:
- File table (~line 560):
rules.py…drift warning - Open Questions Resolved (~line 576): "Dashboard version drift: Packs pin
gnet_revision; migration warns (not errors) on mismatch and still applies the pack."
That's the exact false claim this PR is correcting. Please update those two spots in the rebase so the spec is internally consistent.
… verification script (elastic#350) All 6 dashboard_sha256 pins in curated_packs/registry.yaml were wrong (didn't match a canonical-JSON hash of the pinned grafana.com revision), and gnet_id 11835 pinned gnet_revision 4, which has never existed on grafana.com (that dashboard only has revision 1) -- silently defeating the provenance check these fields exist for. - Correct dashboard_sha256 for the 5 packs whose gnet_revision resolves to a real download; re-pin 11835 to revision 1 with an explicit callout that this can't be independently confirmed to match the pack authors' original source, since revision 4 never existed. - Add scripts/verify_curated_pack_pins.py, a maintainer/CI-only command (network required, not part of `make test`) that re-downloads each pinned revision and re-checks its canonical hash, mirroring the existing fetch_community_corpus.py pattern. Hardened against a malformed download response aborting the whole run, and against an empty/unparseable registry silently reporting a false "0/0 verified" pass. - Document these fields as maintainer-verified provenance pins, not a migration-time gate: a pristine grafana.com download structurally differs from any real Grafana-instance export (mutated id/uid/version), so comparing against operator dashboards would mismatch on every real migration. Rewrote docs/design/curated-dashboard-packs.md's "drift detection" section, which described a revision-comparison mechanism and two CLI commands that were never actually implemented, to match this. - Fix a stale duplicate gnet_revision in the 11835 pack's own fidelity_manifest.yaml left behind by the re-pin, and add a test guarding every pack's manifest against the registry to prevent this drift from recurring silently. Depends on elastic#362 (dropped-source-metric disclosure) for the new docs/sources/grafana.md wording to describe behavior that actually exists on main; should land at or after that PR.
The curated-pack spec's new "no revision comparison" section retracts the migrate-time revision-drift mechanism, but three earlier claims in the same file still asserted the old behavior, leaving the spec contradicting itself: the file table credited `rules.py` with a "drift warning" and `tests/test_curated_packs.py` with "drift detection", and Open Questions Resolved still said migration "warns (not errors) on mismatch". None of that exists: `rules.py` contains no drift code, and those tests cover registry/provenance-pin shape and manifest-vs-registry consistency. Point the drift entry at the real behavior -- provenance pins only, with the per-panel dropped-source-metric disclosure as the actual net. Docs only; no behavior change.
fd31668 to
a5ce6f5
Compare
2 participants
Summary
Issue #350: all 6
dashboard_sha256provenance pins incurated_packs/registry.yamlwere wrong (didn't match a canonical-JSON hash of the pinned grafana.com revision), andgnet_id: 11835pinnedgnet_revision: 4, which has never existed on grafana.com — that dashboard has only ever had revision 1. Together these silently defeated the provenance check these fields exist for.dashboard_sha256for the 5 packs whosegnet_revisionresolves to a real grafana.com download; re-pinned11835to revision 1 (the only real option), with an explicit registry comment noting this can't be independently confirmed to match the pack authors' original source since revision 4 never existed.scripts/verify_curated_pack_pins.py, a maintainer/CI-only command (requires network, intentionally not part ofmake test) that re-downloads each pinned revision from grafana.com and re-checks its canonical hash — mirrors the existingscripts/fetch_community_corpus.pypattern. Ran it live during development: 6/6 pins verify against grafana.com right now.id/uid/version/etc.), so that comparison would mismatch on every real migration. The actual risk (a pack silently missing dashboard content) is instead caught per-panel by the dropped-source-metric detection landing in fix(grafana): disclose source metrics silently dropped from migrated queries #362.registry.yaml's header,docs/sources/grafana.md, anddocs/contributing/dev-commands.md.Code-review fixes (found during a second-model review pass)
docs/design/curated-dashboard-packs.md(linked fromcurated_packs/__init__.pyas the canonical design spec) described a migration-time revision-drift-detection mechanism and two CLI commands (obs-migrate curated-pack check/diff) that were never actually implemented anywhere in the codebase — verifiedresolve_pack_for_dashboarddoes zero revision comparison. Rewrote that section to describe the real current behavior.11835pack's ownfidelity_manifest.yamlstill had the stalegnet_revision: 4left over from before the re-pin (a duplicate, non-runtime-read copy of the same field) — corrected it and added a test that cross-checks every pack's manifest against the registry so this can't drift silently again.verify_curated_pack_pins.py: a malformed/non-JSON download response previously raised an uncaught exception that aborted the entire run instead of failing just that one pack; an empty/unparseable registry previously reported a false "0/0 pins verified" success. Both are now fail-closed, with regression tests.Dependency note
The new
docs/sources/grafana.mdwording describes the per-panel dropped-source-metric detection mechanism, which currently only exists on #362 (not yet merged tomain). This PR should land at or after #362 so that description matches what's actually onmain.Test plan
make test— 5751 passed, 3 skippedmake lint— cleanmake typecheck— cleantests/test_verify_curated_pack_pins.py(new) — offline hashing/parsing/error-handling coverage, no real network callstests/test_curated_packs.py— new registry shape/uniqueness checks plus a manifest-vs-registry consistency guardpython scripts/verify_curated_pack_pins.pyagainst grafana.com — 6/6 pins verified